Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement NativeWasmTypeInto for u32 and u64 #3763

Merged
merged 2 commits into from
May 17, 2023
Merged

Conversation

kajacx
Copy link
Contributor

@kajacx kajacx commented Apr 12, 2023

Implements feature request #3762

Description

Implements NativeWasmTypeInto and NativeWasmType for u32 and u64 on the web target (js feature). See the feature request for more detail.

@kajacx kajacx requested a review from syrusakbary as a code owner April 12, 2023 11:34
@kajacx
Copy link
Contributor Author

kajacx commented Apr 12, 2023

I was not sure what <u32 as NativeWasmType>::Abi should be, so I left it as Self (link). If it should always be the same as the WASM_TYPE, then it probably wouldn't need to be 2 separate lines of code.

But if needs to be i32 to match the WASM_TYPE, I can of course change it. Also, I was not sure how to test it, I did some manual testing, but my I was having some problems with wasm-bindgen, so I couldn't test it as thoroughly as I wanted. Is there an automated testing strategy for something like this? The test would probably need to run in the browser, not sure how that would work.

@kajacx
Copy link
Contributor Author

kajacx commented Apr 16, 2023

I see the PR approved, but after some more testing, I found many things wrong when passing around big u64 values and even big i64 values. I will have to re-check everything and create a minimal reproducible example, but it seems that the BigInt conversion is not working correctly.

This is the part where it goes wrong. I think this was added when I wanted to pass u64 values around for fp-bindgen a few months back, but it doesn't seem to work so well with very large values like u64::MAX - 5 for example.

@kajacx
Copy link
Contributor Author

kajacx commented Apr 20, 2023

Ok, I have prepared a PR to fix a bug about returning negative i32 values from a module on the web, see #3796.

I think this should wait until that is finished.

@ptitSeb ptitSeb added this to the v4.0 milestone May 2, 2023
@kajacx
Copy link
Contributor Author

kajacx commented May 10, 2023

@syrusakbary It seems that this has been already implemented on master in the meantime. See api/src/native_type.rs and types/src/lib.rs. I have only added a test to be sure it stays working.

@ptitSeb ptitSeb merged commit fbe784a into wasmerio:master May 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants